Change `PaginatedResult` `next` / `first` / `current` to be methods by lawrence-forooghian · Pull Request #430 · ably/ably-chat-swift

lawrence-forooghian · 2025-10-15T18:20:11Z

I made these async throwing properties in 20e7f5f, when first translating the public API from JavaScript to Swift. I think it might have just been because of eagerness to use a new language feature. But looking at the proposal that introduced this feature I can't fully convince myself that it's intended for things that go and make an HTTP request. Let's just use methods, for consistency with other platforms.

Summary by CodeRabbit

Refactor
- Pagination APIs have changed from properties to async throwing methods: use next(), first(), and current() instead of next, first, and current.
- Calls now require await and error handling, aligning pagination access with async workflows.
Tests
- Updated tests to invoke the new async methods for pagination.
Chores
- Mocks adjusted to match the updated pagination method signatures.

I made these async throwing properties in 20e7f5f, when first translating the public API from JavaScript to Swift. I think it might have just been because of eagerness to use a new language feature. But looking at the proposal that introduced this feature [1] I can't fully convince myself that it's intended for things that go and make an HTTP request. Let's just use methods, for consistency with other platforms. [1] https://github.com/swiftlang/swift-evolution/blob/main/proposals/0310-effectful-readonly-properties.md

coderabbitai · 2025-10-15T18:20:20Z

Walkthrough

Converted asynchronous throwing computed properties (next, first, current) into async throwing methods across the pagination protocol, wrapper, mocks, and tests. Updated all call sites to invoke methods with parentheses. No other behavioral changes are indicated.

Changes

Cohort / File(s)	Summary
Protocol and Wrapper `Sources/AblyChat/PaginatedResult.swift`	Changed `PaginatedResult` requirements from async throwing properties to `func` methods: `next() -> Self?`, `first() -> Self`, `current() -> Self`. Updated `PaginatedResultWrapper<Item>` to implement the new method-based API with the same async/throwing semantics.
Example Mocks `Example/AblyChatExample/Mocks/Misc.swift`	Replaced public mock properties `next`, `first`, `current` with async throwing methods `next()`, `first()`, `current()`.
Test Mock Implementation `Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift`	Updated `MockPaginatedResult` to match protocol changes: properties → async throwing methods (`next()`, `first()`, `current()`).
Test Call Sites `Tests/AblyChatTests/DefaultMessagesTests.swift`	Adjusted usages from property access to method calls, e.g., `paginatedResult.next` → `paginatedResult.next()`.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor T as Test/Client
  participant P as PaginatedResult (Protocol)
  participant W as PaginatedResultWrapper
  participant S as Underlying SDK
  Note over P,W: Pagination API changed: properties → methods

  T->>P: next() / first() / current() (async)
  activate P
  P->>W: delegate call (async)
  activate W
  W->>S: request page (async)
  alt success
    S-->>W: page/result
    W-->>P: wrapped result
    P-->>T: result (Self / Self?)
  else error
    S-->>W: error (ARTErrorInfo)
    W-->>P: throw ARTErrorInfo
    P-->>T: throw ARTErrorInfo
  end
  deactivate W
  deactivate P

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop from page to page—now I press “go” not “peek,”
With next(), first(), current(), I fetch what I seek.
If errors pop up, I won’t lose my cheer,
I catch ART whispers and keep my path clear.
Async horizons—thump-thump!—the future is sleek. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title accurately and concisely summarizes the main change by stating that `PaginatedResult`’s `next`, `first`, and `current` members are being converted from properties to methods, matching the content of the changeset. It is specific enough for a teammate to understand the intent without unnecessary detail.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch change-PaginatedResult-current-first-next-to-methods

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between efc5265 and 7252a0d.

📒 Files selected for processing (4)

Example/AblyChatExample/Mocks/Misc.swift (1 hunks)
Sources/AblyChat/PaginatedResult.swift (2 hunks)
Tests/AblyChatTests/DefaultMessagesTests.swift (2 hunks)
Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/PaginatedResult.swift

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/DefaultMessagesTests.swift
Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift

🧬 Code graph analysis (4)

Sources/AblyChat/PaginatedResult.swift (3)

Example/AblyChatExample/Mocks/Misc.swift (3)

next (34-34)

first (36-36)

current (38-38)

Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (3)

next (14-14)

first (16-16)

current (18-18)

Sources/AblyChat/InternalError.swift (1)

toARTErrorInfo (19-26)

Example/AblyChatExample/Mocks/Misc.swift (3)

Sources/AblyChat/PaginatedResult.swift (3)

next (77-87)

first (90-100)

current (103-105)

Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (3)

next (14-14)

first (16-16)

current (18-18)

Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (2)

next (46-48)

first (50-52)

Tests/AblyChatTests/DefaultMessagesTests.swift (5)

Example/AblyChatExample/Mocks/Misc.swift (1)

next (34-34)

Sources/AblyChat/PaginatedResult.swift (1)

next (77-87)

Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (1)

next (14-14)

Sources/AblyChat/Messages.swift (1)

next (460-462)

Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)

next (46-48)

Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (3)

Example/AblyChatExample/Mocks/Misc.swift (3)

next (34-34)

first (36-36)

current (38-38)

Sources/AblyChat/PaginatedResult.swift (3)

next (77-87)

first (90-100)

current (103-105)

Sources/AblyChat/Messages.swift (1)

next (460-462)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: lint

🔇 Additional comments (6)

Tests/AblyChatTests/DefaultMessagesTests.swift (1)

468-468: LGTM! Call sites correctly updated to use method syntax.

Both invocations have been properly updated from property access to method calls, consistent with the protocol change.

Also applies to: 522-522

Example/AblyChatExample/Mocks/Misc.swift (1)

34-38: LGTM! Mock correctly updated to match protocol changes.

The method signatures properly conform to the updated PaginatedResult protocol, and the fatalError stubs are appropriate for this mock implementation.

Tests/AblyChatTests/MessageSubscriptionResponseAsyncSequenceTests.swift (2)

1-1: Good addition of missing import.

The import Ably statement is necessary for ARTErrorInfo used in the method signatures below.

14-18: LGTM! Mock correctly updated to match protocol changes.

The method signatures properly conform to the updated PaginatedResult protocol, and the fatalError stubs are appropriate for this mock implementation.

Sources/AblyChat/PaginatedResult.swift (2)

20-24: LGTM! Protocol correctly refactored to use methods instead of properties.

Converting async throwing properties to methods aligns with Swift's design intent for effectful operations, especially those performing HTTP requests. The typed throws with ARTErrorInfo follows the coding guidelines.

77-105: LGTM! Wrapper implementation correctly updated.

The implementation maintains proper error handling:

Uses withCheckedContinuation with Result.get() to bridge typed errors (as per coding guidelines)

Converts InternalError to ARTErrorInfo at the boundary

Follows consistent pattern across next() and first()

current() appropriately returns self with no async work

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

TODO squash once #430 merged

lawrence-forooghian requested review from maratal and umair-ably October 15, 2025 18:20

github-actions Bot deployed to staging/pull/430/AblyChat October 15, 2025 18:21 View deployment

lawrence-forooghian added a commit that referenced this pull request Oct 15, 2025

update for paginated result methods

8fb0d3e

TODO squash once #430 merged

maratal approved these changes Oct 15, 2025

View reviewed changes

lawrence-forooghian merged commit 725ab78 into main Oct 15, 2025
17 checks passed

lawrence-forooghian deleted the change-PaginatedResult-current-first-next-to-methods branch October 15, 2025 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `PaginatedResult` `next` / `first` / `current` to be methods#430

Change `PaginatedResult` `next` / `first` / `current` to be methods#430
lawrence-forooghian merged 1 commit intomainfrom
change-PaginatedResult-current-first-next-to-methods

lawrence-forooghian commented Oct 15, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

lawrence-forooghian commented Oct 15, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

lawrence-forooghian commented Oct 15, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 15, 2025 •

edited

Loading